Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor code cleanup in check_system_module #621

Merged
merged 1 commit into from
May 30, 2024
Merged

Conversation

hoh
Copy link
Member

@hoh hoh commented May 29, 2024

Problem: Reading a file using f.open().read() keeps the file open until the garbage collector deletes the variable.

Since this uses Pathlib already, using Path(...).read_text() is cleaner and does not depend on the opening mode being r.

A file with no content could not be distinguished from a missing file. Instead, this changes the behaviour to return None when the file is missing instead of an empty string.

Problem: Reading a file using `f.open().read()` keeps the file open until the garbage collector deletes the variable.

Since this uses Pathlib already, using `Path(...).read_text()` is cleaner and does not depend on the opening mode being `r`.

A file with no content could not be distinguished from a missing file. Instead, this changes the behaviour to return None when the file is missing instead of an empty string.
@github-actions github-actions bot added the BLUE This PR is simple and straightforward. label May 29, 2024
Copy link

The PR is categorized as 'BLUE' because the changes are relatively small and focused. The PR only includes a few lines of code changes (130 out of 130) and these changes are related to the is_command_available function and check_system_module function. The PR also includes a documentation update for the fix_message_validation function.

Highlighting:

  • The is_command_available function now returns False when the command input is None, which is a small change but could potentially affect the behavior of the program.
  • The check_system_module function now returns None when the system module does not exist, which is a small change but could potentially affect the behavior of the program.
  • The fix_message_validation function now has a docstring describing its purpose and inputs, which is a small change but could potentially improve the readability of the code.

This PR is likely to have minimal impact on the codebase and is unlikely to introduce new bugs. It is recommended to review this PR as soon as possible to ensure that it does not introduce any unexpected behavior.

Copy link

codecov bot commented May 29, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 57.92%. Comparing base (cc3f292) to head (164cd2e).

Files Patch % Lines
src/aleph/vm/utils.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #621   +/-   ##
=======================================
  Coverage   57.92%   57.92%           
=======================================
  Files          60       60           
  Lines        5402     5402           
  Branches      600      600           
=======================================
  Hits         3129     3129           
  Misses       2133     2133           
  Partials      140      140           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@olethanh olethanh requested a review from nesitor May 30, 2024 13:19
Copy link
Collaborator

@olethanh olethanh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm but I will let @nesitor have the final says on the behaviour change

@hoh hoh merged commit 2c2d3f8 into main May 30, 2024
24 checks passed
@Psycojoker Psycojoker deleted the hoh-clean-check-system-module branch July 24, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BLUE This PR is simple and straightforward.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants